feat!: constrain AVM EmitNullifier opcode#15853
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dbanks12
force-pushed
the
db/emitnullifier
branch
from
17eea1e to
3599ca1
Compare
dbanks12
requested review from
IlyasRidhuan,
LeilaWang,
Maddiaa0,
fcarreiro and
jeanmon
as code owners
dbanks12
force-pushed
the
db/emitnullifier
branch
from
3599ca1 to
8da60ea
Compare
|
|
||
| if (context.get_is_static()) { | ||
| throw OpcodeExecutionException("SSTORE: Cannot write to storage in static context"); | ||
| } | ||
|
|
||
| if (!was_slot_written_before && |
There was a problem hiding this comment.
This was missing from these other opcodes.
| NiceMock<MockPoseidon2> poseidon2; | ||
| NiceMock<MockMerkleCheck> merkle_check; | ||
| NiceMock<MockRangeCheck> range_check; | ||
| NiceMock<MockFieldGreaterThan> field_gt; |
There was a problem hiding this comment.
We should migrate these to StrictMocks. It looks like we actually do care about what these things do (looking at the EXPECTs that follow). This will cover us in case there are any missed expectations whose default values might impact our control flow.
FYI there are also now "fakes" (FakePoseidon and FakeGt) that might be helpful for you too.
There was a problem hiding this comment.
I'm happy to switch to StrictMocks. What exactly is the difference?
There was a problem hiding this comment.
I said this in a standup when you were OOO: NiceMock is almost always not what you want. It returns some default value which can change your control flow. StrictMock is what you want. If you are making 0 calls, strictmock will be fine. If you are making > 0 calls, then it is expected that the result of the call modifies your control flow so you want to set an EXPECT_CALL. The only case that might make sense for a NiceMock is when you call some void assert_... which will not change anything, but ideally you can just use StrickMock unless it becomes too verbose.
barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_nullifier.test.cpp
Outdated
Show resolved
Hide resolved
| .WillOnce(Return(siloed_nullifier)) | ||
| .WillOnce(Return(low_leaf_hash)) | ||
| .WillOnce(Return(updated_low_leaf_hash)) | ||
| .WillOnce(Return(new_leaf_hash)); |
There was a problem hiding this comment.
FWIW i think this is a good use of the poseidon2 mock
| uint32_t prev_num_nullifiers_emitted = 2; | ||
|
|
||
| // mock the nullifier > low leaf nullifier to return true | ||
| EXPECT_CALL(field_gt, ff_gt).WillOnce(Return(true)); |
There was a problem hiding this comment.
not necessarily for this PR but looks like a FakeFieldGT might be useful at some point later
dbanks12
force-pushed
the
db/emitnullifier
branch
from
8da60ea to
8427cb6
Compare
dbanks12
force-pushed
the
db/emitnullifier
branch
from
8427cb6 to
c920de5
Compare
dbanks12
force-pushed
the
graphite-base/15853
branch
from
851f8be to
7aca68a
Compare
See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). BEGIN_COMMIT_OVERRIDE feat!: constrain AVM EmitNullifier opcode (#15853) feat(avm)!: ecc add error handling (#15781) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: David Banks <47112877+dbanks12@users.noreply.github.com> Co-authored-by: Ilyas Ridhuan <ilyas@aztecprotocol.com> Co-authored-by: MirandaWood <miranda@aztecprotocol.com>
3 participants
Also adds handling for state-mutation attempts in C++ simulation.
Followup work: